Add pluggable panel action tests #20163
Conversation
stacey-gammon
force-pushed
the
2018-06-19-pluggable-panel-action-test
branch
2 times, most recently
from
f62d5b2
to
1d91c56
Compare
stacey-gammon
force-pushed
the
2018-06-19-pluggable-panel-action-test
branch
from
bf6ccb1
to
8536d8b
Compare
stacey-gammon
force-pushed
the
2018-06-19-pluggable-panel-action-test
branch
2 times, most recently
from
26a14f7
to
1316826
Compare
|
@elastic/kibana-platform - Ideally this infrastructure for the panel actions test would be generalized, so all plugins would have a place to add functional tests (rather than each plugin having it's own folder under
|
|
I do agree that tests within plugins is ideal, and we should work toward that. I'm not aware of there being any good solution for that currently that plays nicely with all of our test tooling and whatnot. We have to remove Even using link: dependencies in plugin-specific package.json files (my preference for static code sharing in the long run) requires consistent static path references. I haven't given this too much thought, but one solution might be shipping mock plugins with the tests in a plugin and then having some mechanism of registering those plugins with the test tooling. Then when we run tests, the tooling could symlink in the relevant plugins to the kibana-extra directory, which is where they will actually be used in Kibana. Ultimately though, I think we should consider testing these plugin boundaries in a totally different way. I think running all of Kibana along with a special plugin just to test a plugin contract is a little crazy. That's basically how we treat all of our functional testing today, but it doesn't have to be that way. The new platform brings more clearly defined boundaries between plugins, core, and even individual services. We should be able to perform functional testing of any given plugin contract programmatically by wiring up the contracts for things it depends on, invoking the lifecycle functions that are relevant for the test, and then verifying the behaviors/side effects of that plugin's contract. |
|
@epixa - I agree that all sounds great for the mid-long term, but can I use |
|
I would also like to see this still as a short term solution (maybe a bit more generalized, but already talked with Stacey that I could generalize that for more plugins, since visualize team has the very same needs). Also until every code lives in the new platform, it will still be months (or years?), so I think not having functional tests on our plugin APIs is not a solution we should go with. We've already had several regression bugs introduced, that weren't caught by unit tests, since you only would have actually got them when booting up the whole Kibana application and causing some side effects in there. I really don't want to wait until we've all code moved to the new platform before I can get reliable tests on that, and before that there could always be sideeffects from the old platform model, that are causing our issues. |
stacey-gammon
changed the title
stacey-gammon
force-pushed
the
2018-06-19-pluggable-panel-action-test
branch
from
9726300
to
66b033a
Compare
|
@timroes We don't have to wait for the new platform to solve this problem. The principles that the new platform was designed for which make functional testing systems without running complete Kibana installs aren't unique to the new platform, and we often are already applying them today. @stacey-gammon What specific things do you want to test here? That new panel actions can be added? That added panel actions render in the dom? If we need to keep the plugin-path around for the sake of testing, we will, even if only for testing. |
|
@epixa That's nice to hear that it would also work without the new platform. Will have to check how that looks in the end. But I think sometimes I want all of Kibana fired up for some types of tests, because we had a couple of issues, that especially only occurred because all of Kibana was there and some side effects from other part of code were causing an issue. Let me also give an answer to the question you asked Stacey, for what I would like to have this tests available (and why I will try to generalize that PR as soon as it passed tests, into something that can run on multiple plugins). The visualization team would require a solution like this to test mainly for the APIs like the visualize loader, that they work fine when using them in a plugin. We want to test if calling the API really leads to the correct rendered result in the browser (so something that we can't cover by unit tests). Also I want to test for some combinations on how to call the API that are available, but we don't use them inside of Kibana that way (e.g. calling the loader for a chart, on a non time-based index, and don't pass a timeRange in there - in Kibana we always pass the time range in there). So that's were these tests would become very handy (and looking at the past issues, we would have been able to most likely prevent 2 blocker issues in the most recent release by having that tests available). The next part why I think that will be very useful is documentation. Since we currently don't have a proper developer documentation system (and even if we would) it's a very good way to write documentation for your API: generate a well documented plugin that shows all the different uses, and link to that in appropriate places. This approach has two major benefits over just documentation (even in case of a proper developer documentation system):
|
|
I have a lot of thoughts on this, but I don't want to block rolling out better test coverage on a higher level conversation about testing. Go ahead and use One thing to keep in mind though: the work has begun to remove the optimizer, so any plugins that you want to run in a non-dev version of Kibana need to be compiled, which is not something we can reliably do in advance until the optimizer is actually removed. Given that our current functional tests can be/are run on real builds, you should pull plugin tests out into a new jenkins job so we can make sure Kibana is running in dev mode. |
stacey-gammon
force-pushed
the
2018-06-19-pluggable-panel-action-test
branch
4 times, most recently
from
0ac98ce
to
0ac94bd
Compare
💔 Build Failed |
| import ReactDOM from 'react-dom'; | ||
|
|
||
| // TODO: Remove once EUI has typing for EuiFlyout and EuiFlyoutBody | ||
| declare module '@elastic/eui' { |
There was a problem hiding this comment.
note: does this really give us any benefit comparing to just ts-ignore like we do for other cases?
We talked about that inline declare module thing with @timroes the other day and obviously the ideal option would be to have types file in "eui" package, even very simple and incomplete types would be better than that, but only if eui owners would commit to keep these typings up to date. There is no need to write them all, just keep eye on existing types during review. Is it something we can do? Who is the owner of this package? @cjcenizal ?
There was a problem hiding this comment.
Soon as we have a new eui package I can get rid of this. I went into eui and added the typings (elastic/eui#1001).
I think @chandlerprall is taking charge of the typescript efforts in EUI. I'm not sure what the plan is, but it seems a lot easier to upkeep if EUI used typescript instead of maintaining separate typings definitions. Perhaps that is the long term plan.
I'll wait for a new eui to submit this PR so I can get rid of this.
There was a problem hiding this comment.
Yep, Chandler owns the code side of EUI now. I agree that converting all of EUI to TS would make the most sense for ease-of-maintenance.
There was a problem hiding this comment.
Converting EUI to TS is the most likely solution. Either way we know we need to properly support TS consumers.
…06-19-pluggable-panel-action-test
I well underestimated the time it'd take to be able to take advantage of some new code in EUI so I take this back. I'd like to not block this on the eui bump which has been hitting it's own test failures (whether flaky or legit, undetermined). If I can get the ci passing today I'd love to be able to merge before I'm out on vacation starting tomorrow. |
💔 Build Failed |
|
Needed to update jest snapshots for inspector panel. Manual inspector looks good to me:
|
💚 Build Succeeded |
…06-19-pluggable-panel-action-test
💔 Build Failed |
…06-19-pluggable-panel-action-test
💚 Build Succeeded |
|
@chrisdavies this one is ready to go if you don't mind giving a review |
| } | ||
|
|
||
| /** | ||
| * An FlyoutSession describes the session of one opened flyout panel. It offers |
| /** | ||
| * Binds the current flyout session to an Angular scope, meaning this flyout | ||
| * session will be closed as soon as the Angular scope gets destroyed. | ||
| * @param {object} scope - And angular scope object to bind to. |
There was a problem hiding this comment.
Typo: And -> An (or The)
| } | ||
|
|
||
| /** | ||
| * Closes the opened flyout as long as it's stil the open one. |
test/panel_actions/config.js
Outdated
| */ | ||
|
|
||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one |
There was a problem hiding this comment.
I think this may be incorrect. I think this is OSS.
test/panel_actions/index.js
Outdated
| */ | ||
|
|
||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one |
| if (activeSession) { | ||
| activeSession.close(); | ||
| } | ||
| const container = getOrCreateContainerElement(); |
There was a problem hiding this comment.
I have a few thoughts about this. Should EuiFlyout itself just coordinate this internally, essentially acting like a singleton and ensuring only one flyout is ever visible at a time? It almost seems like that would be a sensible design.
Alternatively, we could do that ourselves, and keep this inside the React lifecycle by just making it a plain React component. Something like this, maybe:
// Something like this
let currentFlyout = null;
class SingletonFlyout {
componentWillMount() {
if (currentFlyout) {
currentFlyout.close();
}
currentFlyout = {
flyout: this,
close: this.props.onClose,
};
}
componentWillUnmount() {
if (currentFlyout && currentFlyout.flyout === this) {
currentFlyout = null;
}
}
render() {
return (
<EuiFlyout>
{this.props.children}
<EuiFlyout>
);
}
}The advantage there is that consumers can use it like any other React component. It'd be styled to have a fixed position, etc, but could show/ hide based on props / state changes without special cases or whatever.
It'd be consumed like this:
// in some stateful component's render...
render() {
return (
<div>
My usual component.
<button onClick={() => this.setState({ isFlying: true })}>
Fly, little bird!
</button>
{!this.state.isFlying ? null :
<SingletonFlyout onClose={() => this.setState({ isFlying: false })}>
<h1>Hi!</h1>
</SingletonFlyout>
}
</div>
);
}Or something.
You could pass it an Angular scope as an optional param, too, if that's a case that needs handling.
The advantage to either of those ^^^ approaches is that the lifecycle of the flyout is normal React stuff, no special DOM manipulation or possible leaking edge cases.
If we keep the current solution (which is fine by me), it seems like it might be nice to optionally pass the container in, so that callers could pass a container that is managed by React / destroyed by React and not have to worry about cleanup or extraneous references.
There was a problem hiding this comment.
I prefer your first solutions - having flyout be a singleton. I can't think of any good situation where we should have two flyouts.
I would prefer to leave it as is though in this PR, just for the sake of unblocking some things (Tim wants to add inspector tests, I want to extend the panel action tests, and we both want to generalize this approach to all plugins not just panels). I think we'll have to address the flyout changes soon, though, as what should happen if you open a panel action with a flyout while an inspector is open?
💚 Build Succeeded |
* Add pluggable panel action tests * address code review comments * update inspector snapshot * remove temp declared ts module now that eui has EuiFlyout typings * address code comments
Adds a plugin nested in the test folder to be used for testing purposes.
Adds a flyout session object that is very similar to the inspector session object - need to sync with @timroes and maybe design on the best way to move forward with that. Should we allow multiple flyouts open at the same time? Or only one? And if only one (which is what I would lean towards) we should probably refactor out the inspector session to be a more generic flyout session so pluggable panel actions can easily mount a flyout without it having to specifically be an inspector flyout.
flyout's should also be allowed to be modal (like dashboard's
add panel).Similarly I'd like to expose an easy way for panel actions to display a pop up modal, which probably needs a similar session object to ensure there is only one open at any given time.